fix: fix pagination key handling and Base64 decoding #2002
Conversation
|
Warning Rate limit exceeded@Zblocker64 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds support for accepting raw or Base64-encoded pagination keys for Orders, Bids, and Leases queries; attempts raw decode, then Base64-decode+decode, mapping decode errors to appropriate gRPC codes. Preserves SDK NextKey bytes (no prefixing), introduces hasPaginationKey flow, adjusts reverse-search flag for Bids, and removes an isBase64String helper. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Keeper as MarketKeeper
participant SDK as CosmosSDK
rect rgb(250,250,255)
Client->>Keeper: ListOrders/ListBids/ListLeases with Pagination.Key (raw or base64)
end
rect rgb(245,255,240)
Keeper->>Keeper: if Pagination.Key present\n-> try DecodePaginationKey(raw)\n-> if fail: try base64.Decode -> DecodePaginationKey(decoded)\n-> set hasPaginationKey flag accordingly
end
rect rgb(255,250,240)
Keeper->>SDK: perform paginated store query using raw Pagination.Key (if set)
SDK-->>Keeper: results + pageRes.NextKey (raw bytes)
end
rect rgb(240,255,255)
Keeper->>Client: return results and raw NextKey (no prefixing)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/market/keeper/grpc_query.go (1)
200-207: Fix out-of-bounds index in x/market/keeper/grpc_query.golen(unsolicited) is validated to 1; indexing unsolicited[1] will panic — use unsolicited[0].
- if unsolicited[1] == 1 { + if unsolicited[0] == 1 { reverseSearch = true }
🧹 Nitpick comments (2)
x/market/keeper/grpc_query.go (2)
58-66: Base64 detection is heuristic; prefer a decode-and-fallback strategy (and reuse across endpoints).isBase64String can misclassify raw binary that happens to be base64-safe. Instead, try raw DecodePaginationKey first; if it’s invalid, attempt base64 decode and retry. This removes the need for the heuristic and makes behavior consistent. Also apply the same handling in Bids and Leases for client compatibility.
- // Handle Base64 encoded pagination keys - paginationKeyBytes := req.Pagination.Key - if isBase64String(req.Pagination.Key) { - paginationKeyBytes, err = base64.StdEncoding.DecodeString(string(req.Pagination.Key)) - if err != nil { - return nil, status.Error(codes.InvalidArgument, "invalid base64 pagination key") - } - } - - states, searchPrefix, key, _, err = query.DecodePaginationKey(paginationKeyBytes) + // Accept both raw and base64-encoded keys: try raw first, then base64. + paginationKeyBytes := req.Pagination.Key + states, searchPrefix, key, _, err = query.DecodePaginationKey(paginationKeyBytes) + if err != nil { + if decoded, decErr := base64.StdEncoding.DecodeString(string(req.Pagination.Key)); decErr == nil { + states, searchPrefix, key, _, err = query.DecodePaginationKey(decoded) + } + }Also applies to: 562-573
67-70: Return InvalidArgument for bad pagination keys; don’t surface client errors as Internal.Map ErrInvalidPaginationKey to codes.InvalidArgument. This improves API semantics and debuggability.
import ( "context" + "errors" "encoding/base64"- if err != nil { - return nil, status.Error(codes.Internal, err.Error()) - } + if err != nil { + if errors.Is(err, query.ErrInvalidPaginationKey) { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } + return nil, status.Error(codes.Internal, err.Error()) + }Also applies to: 3-9
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/market/keeper/grpc_query.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
x/market/keeper/grpc_query.go (1)
util/query/pagination.go (1)
DecodePaginationKey(16-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: lint
- GitHub Check: release-dry-run
- GitHub Check: coverage
- GitHub Check: build-bins
- GitHub Check: tests
🔇 Additional comments (3)
x/market/keeper/grpc_query.go (3)
140-141: LGTM: stop prefix-prepending NextKey.Keeping the raw Cosmos SDK NextKey avoids corruption and maintains compatibility with subsequent FilteredPaginate calls.
287-288: LGTM: same fix applied to Bids.Not prepending searchPrefix to NextKey here is correct and consistent with Orders.
5-5: Import added appropriately.encoding/base64 is required for page-key handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
x/market/keeper/grpc_query.go (1)
101-103: Always clear key when advancing to the next state to recompute searchPrefix.Gating on hasPaginationKey stalls cross-state iteration and can cause duplicates; next state reuses previous prefix. Clear unconditionally.
Apply this diff:
- if idx > 0 && !hasPaginationKey { - req.Pagination.Key = nil - } + if idx > 0 { + req.Pagination.Key = nil + }
🧹 Nitpick comments (3)
x/market/keeper/grpc_query.go (3)
53-67: Base64 fallback: also accept raw/URL-safe variants and reduce duplication.Some clients emit RawStd or URL-safe Base64. Broaden acceptance and reuse across Orders/Bids/Leases.
Apply this diff to broaden decoding here (repeat same pattern in Bids/Leases decode blocks):
- if err != nil { - if decoded, decErr := base64.StdEncoding.DecodeString(string(req.Pagination.Key)); decErr == nil { - states, searchPrefix, key, _, err = query.DecodePaginationKey(decoded) - } - } + if err != nil { + // Try multiple Base64 alphabets (std/raw/url). + decoders := []*base64.Encoding{ + base64.StdEncoding, base64.RawStdEncoding, + base64.URLEncoding, base64.RawURLEncoding, + } + for _, enc := range decoders { + if decoded, decErr := enc.DecodeString(string(req.Pagination.Key)); decErr == nil { + states, searchPrefix, key, _, err = query.DecodePaginationKey(decoded) + if err == nil { + break + } + } + } + }
197-205: Broaden Base64 handling here too (mirror Orders).Adopt the multi-decoder fallback to accept std/raw/url Base64.
Apply this diff:
- if err != nil { - if decoded, decErr := base64.StdEncoding.DecodeString(string(req.Pagination.Key)); decErr == nil { - states, searchPrefix, key, unsolicited, err = query.DecodePaginationKey(decoded) - } - } + if err != nil { + decoders := []*base64.Encoding{ + base64.StdEncoding, base64.RawStdEncoding, + base64.URLEncoding, base64.RawURLEncoding, + } + for _, enc := range decoders { + if decoded, decErr := enc.DecodeString(string(req.Pagination.Key)); decErr == nil { + states, searchPrefix, key, unsolicited, err = query.DecodePaginationKey(decoded) + if err == nil { + break + } + } + } + }
363-370: Mirror Base64 multi-decoder fallback in Leases.Keep parity with Orders/Bids for client compatibility.
Apply this diff:
- if err != nil { - if decoded, decErr := base64.StdEncoding.DecodeString(string(req.Pagination.Key)); decErr == nil { - states, searchPrefix, key, unsolicited, err = query.DecodePaginationKey(decoded) - } - } + if err != nil { + decoders := []*base64.Encoding{ + base64.StdEncoding, base64.RawStdEncoding, + base64.URLEncoding, base64.RawURLEncoding, + } + for _, enc := range decoders { + if decoded, decErr := enc.DecodeString(string(req.Pagination.Key)); decErr == nil { + states, searchPrefix, key, unsolicited, err = query.DecodePaginationKey(decoded) + if err == nil { + break + } + } + } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/market/keeper/grpc_query.go(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
x/market/keeper/grpc_query.go (1)
util/query/pagination.go (2)
DecodePaginationKey(16-89)ErrInvalidPaginationKey(12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: lint
- GitHub Check: release-dry-run
- GitHub Check: coverage
- GitHub Check: build-macos
- GitHub Check: tests
- GitHub Check: build-bins
🔇 Additional comments (5)
x/market/keeper/grpc_query.go (5)
5-6: LGTM on new imports.Required for Base64 fallback and error checks.
142-142: LGTM: Keep raw SDK NextKey (no prefixing).Prevents key corruption across page boundaries.
218-221: LGTM: reverseSearch flag carried via unsolicited[0].Consistent with Encode/Decode contract.
246-248: LGTM: clear key on state transitions.Prevents stale prefix reuse during multi-state scans.
411-413: LGTM: clear key across state buckets.Ensures correct prefix recomputation per state.
|
closing due to new PR with a new ReadPageRequest function |
Pull request was closed
Description
Closes: #XXXX
This PR fixes critical pagination bugs in the market query handlers that were causing providers to see duplicate orders during catchup operations. The issues were:
Pagination key reset bug: The
req.Pagination.Keywas being reset tonilfor subsequent state iterations (idx > 0), causing pagination to restart from the beginning instead of continuing sequentially.Base64 encoding/decoding mismatch: CLI was passing Base64-encoded pagination keys, but the server expected binary data, leading to checksum validation failures.
SearchPrefix corruption: The raw Cosmos SDK
nextKeywas being corrupted by prependingsearchPrefix, making it incompatible withFilteredPaginateon subsequent requests.Key Changes:
if idx > 0 && !hasPaginationKeyTesting:
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.md